Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli.config: implement set and unset subcommands #1986

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 9, 2020

Description

If @Exirel can submit PRs that weren't "requested" by an issue, then so can I! 😛

Honestly, this was going to happen sooner or later. It's the next natural evolution of the sopel-config CLI.

Why now? I was working on an out-of-tree plugin and kept needing to edit the config file due to shortcomings in the configuration wizard UX (namely, you can't unset a value through a prompt in sopel-plugins configure <plugin_name>).

While I've never been terribly fond of how the configuration wizard works—goodness knows it needs an overhaul—fixing that was too big of a project to start late on a Sunday night. This, by comparison, took less than half an hour to build out and (manually) test. Since we don't have any unit tests for large portions of the CLI module (other than cli.run and cli.utils), I didn't worry about adding any for these new subcommands just yet.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@dgw dgw added the Feature label Nov 9, 2020
@dgw dgw added this to the 7.1.0 milestone Nov 9, 2020
@dgw dgw requested a review from a team November 9, 2020 06:02
sopel/cli/config.py Outdated Show resolved Hide resolved
Note that an invalid value already present in the config file will block
using `sopel-config` commands **at all** for now. This is not new.

Plugin-defined `StaticSection`s aren't loaded as such here. Only values
present in the config file are parsed by `configparser`, and new values
can't be validated unless `cli.utils.load_settings()` is reworked to
load plugins' static sections too.
@dgw dgw requested a review from Exirel November 9, 2020 18:34
@Exirel
Copy link
Contributor

Exirel commented Jan 11, 2021

OK, I know I should have review that a long time ago, but I wanted to have an actual opinion, and not just doubt or gut feeling (positive or negative). I apologize because I'm going to ask for more work and it's kind of my fault.

So, we both know the problem: at this stage, the command doesn't know the real configuration, i.e. plugins' static sections. We assumed that we couldn't get them, and for my part that's because I thought the config might not be viable at this stage. And I think this is wrong—and partially my fault.

When using set/unset, I think we should assume that the config is viable enough to, at least, load & setup plugins. Then set/unset can validate the new value.

Now, what if "hey I manually edited and now it's not valid what do I do?"... then I'll say: add a flag to deactivate validation (by skipping loading I guess?).

What do you say?

@dgw
Copy link
Member Author

dgw commented Jan 17, 2021

What I'd like is a way to load only the config sections of plugins, but (correct me if I'm forgetting something) that's not possible with the current loader architecture, plugin structure, nor how Python loads things in general. AFAICT we're stuck loading all plugins and running their setup() functions because that's where the config sections get defined.*

So with that out of the way, yes. I agree in principle that set/unset should load and setup plugins so their values can be validated, and there should be a way to bypass that step if necessary (with a big WARNING: printed to the console when used).

I'd also like to handle the case where ConfigParser can't load the file because of some syntax error, but that might be much more work than it's worth.

In any case, I think this PR is best left alone for now and pushed to 8.0. Agree? (You can re-milestone and assign me if you do. 😁)


* — So I did think of an alternative: Call each plugin's configure() function instead of setup(). But that's not practical because 1) configure() isn't required to use config values, but setup() is; and 2) it would require stubbing out or monkeypatching a bunch of other stuff in the config/section objects, which is arguably messier than just loading the whole plugin.

@Exirel
Copy link
Contributor

Exirel commented Jan 17, 2021

What I'd like is a way to load only the config sections of plugins, but (correct me if I'm forgetting something) that's not possible with the current loader architecture, plugin structure

Yup, that's the problem. I'd like to fix it too, but that's not currently possible without changes. And even then, the solution won't be perfect because we can't prevent someone from defining sections from the setup hook.

  • — So I did think of an alternative

Got the same idea, with the same conclusion. What I want to do in a future version, is to separate the steps:

  • get metadata (see below)
  • define config
  • setup
  • configure
  • shutdown

Get metadata: that could be a way to inform about the plugins without having to run any more code than necessary, i.e. something that returns the author, short & long description, and a list of (section name, config class), which could come in handy. But that's future talk!

@Exirel Exirel modified the milestones: 7.1.0, 8.0.0 Jan 17, 2021
@dgw dgw marked this pull request as draft June 30, 2021 03:48
@dgw
Copy link
Member Author

dgw commented Jun 30, 2021

This should have been dropped back to draft status weeks ago. Or really, about 5 months back.

@dgw dgw mentioned this pull request Dec 23, 2021
5 tasks
@dgw dgw removed this from the 8.0.0 milestone Mar 2, 2023
@dgw dgw added the Stale Mostly used for PRs that no longer work and need updating before re-review/merge. label Mar 2, 2023
@dgw
Copy link
Member Author

dgw commented Mar 2, 2023

Setting some 8.0 priorities. This can go in a minor 8.x release, or 9.0, depending. It's a new feature with no accompanying deprecations that shouldn't affect the behavior of anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Stale Mostly used for PRs that no longer work and need updating before re-review/merge. Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants